Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use POSIX mode for large numbers in tar archives (#4384) #4388

Merged
merged 2 commits into from
Sep 8, 2021
Merged

Use POSIX mode for large numbers in tar archives (#4384) #4388

merged 2 commits into from
Sep 8, 2021

Conversation

schmidt-galen-heb
Copy link
Contributor

Resolves #4384

@batt842
Copy link

batt842 commented Aug 21, 2021

Looks like there is one more place that uses TarArchiveOutputStream, it's org.testcontainers.images.builder.ImageFromDockerfile:124. But I'm not sure how to test it or whether it should be done at this time.
@kiview What do you think?

@kiview
Copy link
Member

kiview commented Aug 23, 2021

Thanks for rasing this PR @schmidt-galen-heb and thanks for pointing out the other relevant part in the code @batt842.

Could any of you run this test in your environment, so we see if we need the fix at this location as well?

@schmidt-galen-heb
Copy link
Contributor Author

Thanks for pointing that out. I ran the test with commons-compress 1.21 and it passed, however I still think that location needs to be updated as well.

In investigating that test, I was able to determine why the build started failing in 1.21:

From the original stacktrace:

Caused by: java.lang.IllegalArgumentException: user id '<redacted>' is too big ( > 2097151 ).
	at org.apache.commons.compress.archivers.tar.TarArchiveOutputStream.failForBigNumber(TarArchiveOutputStream.java:651)
	at org.apache.commons.compress.archivers.tar.TarArchiveOutputStream.failForBigNumber(TarArchiveOutputStream.java:639)
	at org.apache.commons.compress.archivers.tar.TarArchiveOutputStream.failForBigNumbers(TarArchiveOutputStream.java:630)
	at org.apache.commons.compress.archivers.tar.TarArchiveOutputStream.putArchiveEntry(TarArchiveOutputStream.java:377)
	at org.testcontainers.utility.MountableFile.recursiveTar(MountableFile.java:327)
	at org.testcontainers.utility.MountableFile.transferTo(MountableFile.java:304)
	at org.testcontainers.containers.ContainerState.copyFileToContainer(ContainerState.java:276)
	at org.testcontainers.containers.ContainerState.copyFileToContainer(ContainerState.java:253)
	at java.base/java.util.LinkedHashMap.forEach(LinkedHashMap.java:684)
	at org.testcontainers.containers.GenericContainer.tryStart(GenericContainer.java:414)
	... 39 more

MountableFile uses the TarArchiveEntry(File, String) constructor to create the archive entry. In 1.20, this did not populate the user ID property, however in 1.21, they added a call to readOsSpecificProperties(...), which does populate the user ID.

So although the test passes, if a user were to call ImageFromDockerfile.withFileFromTransferable(...) with a MountableFile, it would fail.

@schmidt-galen-heb
Copy link
Contributor Author

@kiview Anything else you'd like from my side on this?

@kiview
Copy link
Member

kiview commented Aug 31, 2021

@schmidt-galen-heb
Thanks a lot, LGTM. Let the others have a look and then we merge 👍

@rnorth
Copy link
Member

rnorth commented Sep 6, 2021

This looks fine, but I'm confused by this comment by @schmidt-galen-heb:

So although the test passes, if a user were to call ImageFromDockerfile.withFileFromTransferable(...) with a MountableFile, it would fail.

Are you saying that this problem still exists, or it did exist but is now fixed? 😄

@schmidt-galen-heb
Copy link
Contributor Author

Are you saying that this problem still exists, or it did exist but is now fixed? 😄

Sorry, it did exist, but is fixed by the change in ImageFromDockerfile.java

@rnorth
Copy link
Member

rnorth commented Sep 7, 2021

Are you saying that this problem still exists, or it did exist but is now fixed? 😄

Sorry, it did exist, but is fixed by the change in ImageFromDockerfile.java

Great! Thanks for clarifying.

@rnorth rnorth merged commit 8746f20 into testcontainers:master Sep 8, 2021
@kiview kiview added this to the next milestone Oct 13, 2021
@jayleenli
Copy link

Currently still having this issue with version 1.16.1, was the fix pushed to the next release?

@kiview
Copy link
Member

kiview commented Oct 22, 2021

Hey @jayleenli, sorry to hear that, the fix is included in 1.16.1.

@schmidt-galen-heb @batt842 Can you verify, that the bug as you observed it is fixed in 1.16.1?

@schmidt-galen-heb
Copy link
Contributor Author

Hey @kiview, I can confirm that upgrading to 1.16.1 resolved the issue for me

@batt842
Copy link

batt842 commented Oct 28, 2021

@kiview It works for me too.
Thanks @schmidt-galen-heb

@abialas
Copy link

abialas commented Nov 25, 2021

I still have this issue on version 1.16.2 and MacOS:

Caused by: org.testcontainers.containers.ContainerFetchException: Can't get Docker image: RemoteDockerImage(imageName=, imagePullPolicy=DefaultPullPolicy())
at org.testcontainers.containers.GenericContainer.getDockerImageName(GenericContainer.java:1326)
at org.testcontainers.containers.GenericContainer.logger(GenericContainer.java:643)
at org.testcontainers.containers.GenericContainer.doStart(GenericContainer.java:326)
... 36 more
Caused by: java.lang.IllegalArgumentException: group id '34548085' is too big ( > 2097151 ). Use STAR or POSIX extensions to overcome this limit

@murat3
Copy link

murat3 commented Dec 8, 2021

I can confirm this issue still exist in 1.16.2 on MacOS

Caused by: java.lang.IllegalArgumentException: group id '5361194532 is too big ( > 2097151 ). Use STAR or POSIX extensions to overcome this limit
at org.apache.commons.compress.archivers.tar.TarArchiveOutputStream.failForBigNumber(TarArchiveOutputStream.java:651)
at org.apache.commons.compress.archivers.tar.TarArchiveOutputStream.failForBigNumberWithPosixMessage(TarArchiveOutputStream.java:644)
at org.apache.commons.compress.archivers.tar.TarArchiveOutputStream.failForBigNumbers(TarArchiveOutputStream.java:626)
at org.apache.commons.compress.archivers.tar.TarArchiveOutputStream.putArchiveEntry(TarArchiveOutputStream.java:377)
at org.testcontainers.shaded.com.github.dockerjava.core.util.CompressArchiveUtil.addFileToTar(CompressArchiveUtil.java:41)
at org.testcontainers.shaded.com.github.dockerjava.core.util.CompressArchiveUtil.archiveTARFiles(CompressArchiveUtil.java:106)
at org.testcontainers.shaded.com.github.dockerjava.core.dockerfile.Dockerfile$ScannedResult.buildDockerFolderTar(Dockerfile.java:134)
at org.testcontainers.shaded.com.github.dockerjava.core.dockerfile.Dockerfile$ScannedResult.buildDockerFolderTar(Dockerfile.java:124)
at org.testcontainers.shaded.com.github.dockerjava.core.command.BuildImageCmdImpl.withDockerfile(BuildImageCmdImpl.java:346)
at org.testcontainers.shaded.com.github.dockerjava.core.command.BuildImageCmdImpl.withDockerfile(BuildImageCmdImpl.java:23)
at org.testcontainers.images.builder.ImageFromDockerfile.lambda$configure$0(ImageFromDockerfile.java:154)
at java.util.Optional.ifPresent(Optional.java:159)
at org.testcontainers.images.builder.ImageFromDockerfile.configure(ImageFromDockerfile.java:153)
at org.microshed.testing.testcontainers.internal.ImageFromDockerfile.configure(ImageFromDockerfile.java:45)
at org.testcontainers.images.builder.ImageFromDockerfile.resolve(ImageFromDockerfile.java:109)
at org.testcontainers.images.builder.ImageFromDockerfile.resolve(ImageFromDockerfile.java:37)
at org.testcontainers.utility.LazyFuture.getResolvedValue(LazyFuture.java:17)
at org.testcontainers.utility.LazyFuture.get(LazyFuture.java:39)
at org.testcontainers.shaded.com.google.common.util.concurrent.Futures$3.get(Futures.java:1332)
at org.testcontainers.images.RemoteDockerImage.getImageName(RemoteDockerImage.java:115)
at org.testcontainers.images.RemoteDockerImage.resolve(RemoteDockerImage.java:64)
at org.testcontainers.images.RemoteDockerImage.resolve(RemoteDockerImage.java:28)
at org.testcontainers.utility.LazyFuture.getResolvedValue(LazyFuture.java:17)
at org.testcontainers.utility.LazyFuture.get(LazyFuture.java:39)
at org.testcontainers.containers.GenericContainer.getDockerImageName(GenericContainer.java:1324)

@schmidt-galen-heb
Copy link
Contributor Author

From your stack trace, the issue is actually in docker-java and not testcontainers.

Specifically, the same changes from this PR will need to be applied to CompressArchiveUtil.java, and once released, testcontainers will need to be updated to the new version of docker-java

@murat3
Copy link

murat3 commented Dec 8, 2021

Yes indeed. I created a new issue on docker-java:
docker-java/docker-java#1747

@todor-kolev
Copy link

Yes indeed. I created a new issue on docker-java: docker-java/docker-java#1747

Is there a workaround of any sort, do you know?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

user id 'xxx' is too big ( > 2097151 ) when using commons-compress 1.21 with large user ID
9 participants